-
Notifications
You must be signed in to change notification settings - Fork 128
Connection State: Error Handling + Cleaning / refactoring / #3521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection State: Error Handling + Cleaning / refactoring / #3521
Conversation
Updated the test because now name will be the userId instead of default display name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work with fixing things up as you go. For the sake of getting our two branches merged into one as soon as possible, I went ahead and applied some changes directly for things which seemed hopefully uncontrovertial - if you like you can have a look through them. The rest of my comments are mostly about things from my branch that seem to have been lost in the previous merge conflicts.
I did also confirm that the app runs and connects to an SFU successfully.
scope.onEnd(() => { | ||
this.muteStates.audio.unsetHandler(); | ||
this.muteStates.video.unsetHandler(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 86fb026 I moved these calls to unsetHandler
to the stop
method as scope.onEnd
happens much later than what we need for SFU-hopping. I believe they should stay there in stop
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, scratch that, we should just give the Connection class a proper ObservableScope of its own that ends at the right time (when CallViewModel is about to drop the Connection object).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can work on that.
useCallViewKeyboardShortcuts() changed a param from `setMicrophoneMuted` to `setAudioEnabled`, the boolean arg of the callback is inverse tht it used to be
Signed-off-by: Timo K <[email protected]>
0fd4143
to
de5f519
Compare
c846ea6
into
voip-team/rebased-multiSFU
Better reviewed commit per commit